Closed
Conversation
guybedford
approved these changes
Nov 21, 2018
devsnek
approved these changes
Nov 21, 2018
devsnek
reviewed
Nov 21, 2018
lib/internal/modules/cjs/loader.js
Outdated
Member
There was a problem hiding this comment.
i think this needs to be something like
const exports = this.exports;
module.modulePromise.then(({ reflect }) => {
reflect.exports.default.set(exports);
});testing locally atm
Member
|
@MylesBorins this passes: diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
index df270db69d..23bd8badbb 100644
--- a/lib/internal/modules/cjs/loader.js
+++ b/lib/internal/modules/cjs/loader.js
@@ -623,23 +623,24 @@ Module.prototype.load = function(filename) {
if (experimentalModules) {
if (asyncESM === undefined) lazyLoadESM();
const ESMLoader = asyncESM.ESMLoader;
- const url = pathToFileURL(filename);
- const urlString = `${url}`;
+ const url = `${pathToFileURL(filename)}`;
+ const module = ESMLoader.moduleMap.get(url);
+ // create module entry at load time to snapshot exports correctly
const exports = this.exports;
- if (ESMLoader.moduleMap.has(urlString) !== true) {
+ if (module !== undefined) { // called from cjs translator
+ module.reflect.onReady((reflect) => {
+ reflect.exports.default.set(exports);
+ });
+ } else { // preemptively cache
ESMLoader.moduleMap.set(
- urlString,
+ url,
new ModuleJob(ESMLoader, url, async () => {
- const ctx = createDynamicModule(
- ['default'], url);
- ctx.reflect.exports.default.set(exports);
- return ctx;
+ return createDynamicModule(
+ ['default'], url, (reflect) => {
+ reflect.exports.default.set(exports);
+ });
})
);
- } else {
- const job = ESMLoader.moduleMap.get(urlString);
- if (job.reflect)
- job.reflect.exports.default.set(exports);
}
}
};
diff --git a/lib/internal/modules/esm/create_dynamic_module.js b/lib/internal/modules/esm/create_dynamic_module.js
index 8e93a08502..9afc866e40 100644
--- a/lib/internal/modules/esm/create_dynamic_module.js
+++ b/lib/internal/modules/esm/create_dynamic_module.js
@@ -1,6 +1,6 @@
'use strict';
-const { ModuleWrap } = internalBinding('module_wrap');
+const { ModuleWrap, callbackMap } = internalBinding('module_wrap');
const debug = require('util').debuglog('esm');
const ArrayJoin = Function.call.bind(Array.prototype.join);
const ArrayMap = Function.call.bind(Array.prototype.map);
@@ -10,50 +10,47 @@ const createDynamicModule = (exports, url = '', evaluate) => {
`creating ESM facade for ${url} with exports: ${ArrayJoin(exports, ', ')}`
);
const names = ArrayMap(exports, (name) => `${name}`);
- // Create two modules: One whose exports are get- and set-able ('reflective'),
- // and one which re-exports all of these but additionally may
- // run an executor function once everything is set up.
- const src = `
- export let executor;
- ${ArrayJoin(ArrayMap(names, (name) => `export let $${name};`), '\n')}
- /* This function is implicitly returned as the module's completion value */
- (() => ({
- setExecutor: fn => executor = fn,
- reflect: {
- exports: { ${
- ArrayJoin(ArrayMap(names, (name) => `
- ${name}: {
- get: () => $${name},
- set: v => $${name} = v
- }`), ', \n')}
- }
- }
- }));`;
- const reflectiveModule = new ModuleWrap(src, `cjs-facade:${url}`);
- reflectiveModule.instantiate();
- const { setExecutor, reflect } = reflectiveModule.evaluate(-1, false)();
- // public exposed ESM
- const reexports = `
- import {
- executor,
- ${ArrayMap(names, (name) => `$${name}`)}
- } from "";
- export {
- ${ArrayJoin(ArrayMap(names, (name) => `$${name} as ${name}`), ', ')}
- }
- if (typeof executor === "function") {
- // add await to this later if top level await comes along
- executor()
- }`;
- if (typeof evaluate === 'function') {
- setExecutor(() => evaluate(reflect));
- }
- const module = new ModuleWrap(reexports, `${url}`);
- module.link(async () => reflectiveModule);
- module.instantiate();
- reflect.namespace = module.namespace();
+
+ const source = `
+${ArrayJoin(ArrayMap(names, (name) =>
+ `let $${name};
+export { $${name} as ${name} };
+import.meta.exports.${name} = {
+ get: () => $${name},
+ set: (v) => $${name} = v,
+};`), '\n')
+}
+
+import.meta.done();
+`;
+
+ const m = new ModuleWrap(source, `${url}`);
+ m.link(() => 0);
+ m.instantiate();
+
+ const readyfns = new Set();
+ const reflect = {
+ namespace: m.namespace(),
+ exports: {},
+ onReady: (cb) => { readyfns.add(cb); },
+ };
+
+ callbackMap.set(m, {
+ initializeImportMeta: (meta, wrap) => {
+ meta.exports = reflect.exports;
+ meta.done = () => {
+ evaluate(reflect);
+ reflect.onReady = (cb) => cb(reflect);
+ for (const fn of readyfns) {
+ readyfns.delete(fn);
+ fn(reflect);
+ }
+ };
+ },
+ });
+
return {
- module,
+ module: m,
reflect,
};
};
diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js
index 0c34283b8a..0d19a728aa 100644
--- a/lib/internal/modules/esm/translators.js
+++ b/lib/internal/modules/esm/translators.js
@@ -60,9 +60,10 @@ translators.set('cjs', async (url, isMain) => {
const module = CJSModule._cache[
isWindows ? StringReplace(pathname, winSepRegEx, '\\') : pathname];
if (module && module.loaded) {
- const ctx = createDynamicModule(['default'], url);
- ctx.reflect.exports.default.set(module.exports);
- return ctx;
+ const exports = module.exports;
+ return createDynamicModule(['default'], url, (reflect) => {
+ reflect.exports.default.set(exports);
+ });
}
return createDynamicModule(['default'], url, () => {
debug(`Loading CJSModule ${url}`); |
Contributor
Author
|
I'm very confused as to why this patch failed on master but CI passed without an issue on ecma-script modules (which was recently rebased against master) original pr: nodejs/ecmascript-modules#9 |
Member
|
@MylesBorins because downstream removed all the important tests for it, because we removed interop. |
This is a change from the ecmascript-modules fork. There is no change to behavior and we would like to upstream to reduce the delta between our repos. Refs: https://github.com/nodejs/ecmascript-modules#9
7267f19 to
33f7be6
Compare
Contributor
Author
|
Updated to the newer patch you shared above |
MylesBorins
added a commit
to nodejs/ecmascript-modules
that referenced
this pull request
Nov 30, 2018
This is a change from the ecmascript-modules fork. There is no change to behavior and we would like to upstream to reduce the delta between our repos. Refs: https://github.com/nodejs/ecmascript-modules#9 PR-URL: nodejs/node#24560 Refs: #9 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
Contributor
Author
|
landed in 2931c50 |
BridgeAR
pushed a commit
that referenced
this pull request
Dec 5, 2018
This is a change from the ecmascript-modules fork. There is no change to behavior and we would like to upstream to reduce the delta between our repos. Refs: https://github.com/nodejs/ecmascript-modules#9 PR-URL: #24560 Refs: nodejs/ecmascript-modules#9 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
This was referenced Dec 7, 2018
refack
pushed a commit
to refack/node
that referenced
this pull request
Jan 14, 2019
This is a change from the ecmascript-modules fork. There is no change to behavior and we would like to upstream to reduce the delta between our repos. Refs: https://github.com/nodejs/ecmascript-modules#9 PR-URL: nodejs#24560 Refs: nodejs/ecmascript-modules#9 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
BethGriggs
pushed a commit
that referenced
this pull request
Feb 12, 2019
This is a change from the ecmascript-modules fork. There is no change to behavior and we would like to upstream to reduce the delta between our repos. Refs: https://github.com/nodejs/ecmascript-modules#9 PR-URL: #24560 Refs: nodejs/ecmascript-modules#9 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
Merged
rvagg
pushed a commit
that referenced
this pull request
Feb 28, 2019
This is a change from the ecmascript-modules fork. There is no change to behavior and we would like to upstream to reduce the delta between our repos. Refs: https://github.com/nodejs/ecmascript-modules#9 PR-URL: #24560 Refs: nodejs/ecmascript-modules#9 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a change from the ecmascript-modules fork.
There is no change to behavior and we would like to
upstream to reduce the delta between our repos.
Refs: nodejs/ecmascript-modules#9